test(registration-service): expose metrics for the git commit#1269
Open
xcoulon wants to merge 6 commits intocodeready-toolchain:masterfrom
Open
test(registration-service): expose metrics for the git commit#1269xcoulon wants to merge 6 commits intocodeready-toolchain:masterfrom
xcoulon wants to merge 6 commits intocodeready-toolchain:masterfrom
Conversation
xcoulon
force-pushed
the
prometheus_metric_commit
branch
from
cf06267 to
482a363
Compare
WalkthroughExpose per-service metrics routes with explicit port/TLS, log computed metrics URLs, parameterize metric-fetching to accept a metrics URL, add registration-service metrics handling, extend HostAwaitility with a registration metrics URL and new metric constants, and update tests to use per-instance metrics endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Wait as Awaitility
participant Route as OpenShift Route/Ingress
participant HostMetrics as host-operator-metrics-service
participant RegMetrics as registration-service-metrics
participant MemberOp as member-operator-instance
rect rgba(200,200,255,0.5)
Test->>Wait: SetupRouteForService(service, path, port, tls)
Wait->>Route: create/ensure Route with spec.Port & spec.TLS
end
rect rgba(200,255,200,0.5)
Test->>Wait: WaitForRouteToBeAvailable(ns,name,path)
Wait->>Route: poll route host
Route-->>Wait: host ready
Wait-->>Test: route host returned
end
rect rgba(255,200,200,0.5)
Test->>HostMetrics: GET {hostMetricsURL}/metrics
HostMetrics-->>Test: Prometheus metrics
Test->>RegMetrics: GET {regSvcMetricsURL}/metrics
RegMetrics-->>Test: Prometheus metrics
Test->>MemberOp: GET {memberMetricsURL}/metrics
MemberOp-->>Test: Prometheus metrics
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
also, test new `commit` and `short_commit` metrics for host and member operators Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
xcoulon
force-pushed
the
prometheus_metric_commit
branch
from
482a363 to
a75907b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/metrics/metrics_test.go (1)
74-75:⚠️ Potential issue | 🟠 MajorBug:
member2Awaitis incorrectly assignedMember1()instead ofMember2().This appears to be a copy-paste error. Both
member1Awaitandmember2Awaitare assigned the same member operator, which means the test is effectively testing the same member twice rather than two different members.Proposed fix
// given member1Await := awaitilities.Member1() - member2Await := awaitilities.Member1() + member2Await := awaitilities.Member2()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 74 - 75, The test mistakenly assigns member2Await using awaitilities.Member1() instead of awaitilities.Member2(), causing both variables to refer to the same member; update the assignment of member2Await to call awaitilities.Member2() so the test exercises two distinct members (change the second line where member2Await is defined).
🧹 Nitpick comments (4)
testsupport/init.go (1)
209-245: Consider extracting the duplicated"/metrics"path literal.SonarCloud flagged this literal as duplicated 3 times. While the code is functional, extracting it to a constant would improve maintainability.
Suggested fix
Add a constant at the top of the function or file:
const metricsPath = "/metrics"Then use it in the route setup calls:
- hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics", + hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", metricsPath,Apply similarly for lines 222 and 235.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/init.go` around lines 209 - 245, Extract the duplicated "/metrics" literal into a single constant (e.g., const metricsPath = "/metrics") at the top of the function or file, then replace the three usages in the SetupRouteForService calls for initHostAwait.SetupRouteForService (host metrics), initHostAwait.SetupRouteForService (registration service metrics), and initMemberAwait.SetupRouteForService (member metrics) with that constant to remove duplication and improve maintainability.testsupport/metrics/metrics.go (1)
45-45: Inconsistent HTTP method casing.This line still uses
"Get"while line 139 was updated to"GET". While Go'shttp.NewRequestis case-insensitive for method names, using the canonical uppercase form is preferred for consistency and clarity.Suggested fix
- request, err := http.NewRequest("Get", uri, nil) + request, err := http.NewRequest("GET", uri, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/metrics/metrics.go` at line 45, The HTTP method string in the http.NewRequest call uses "Get" inconsistent with the canonical uppercase form; update the call in testsupport/metrics/metrics.go (the http.NewRequest invocation that assigns to request) to use "GET" instead of "Get" so it matches the other usage (e.g., the one on line 139) and keeps method casing consistent across the codebase.test/metrics/metrics_test.go (2)
102-107: Unnecessary/incorrect route wait for registration-service subtest.The test waits for the
host-operator-metrics-serviceroute but then useshostAwait.RegistrationServiceMetricsURLwhich is set up from a different route (registration-service-metrics) ininit.go. This wait is redundant sinceWaitForDeploymentsalready ensures the registration service metrics route is available.Suggested fix - remove unnecessary wait
t.Run("registration-service", func(t *testing.T) { - // given hostAwait := awaitilities.Host() - _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") - require.NoError(t, err) t.Run("commit", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 102 - 107, In the "registration-service" subtest remove the redundant call to hostAwait.WaitForRouteToBeAvailable that waits for "host-operator-metrics-service" because the test uses hostAwait.RegistrationServiceMetricsURL (which is derived from the "registration-service-metrics" route in init.go) and WaitForDeployments already guarantees the route is ready; update the subtest to rely on hostAwait.RegistrationServiceMetricsURL directly and delete the WaitForRouteToBeAvailable invocation and its error handling.
38-38: Consider extracting duplicated literals per SonarCloud findings.The string literals
"/metrics"and"host-operator-metrics-service"are duplicated multiple times. Consider defining constants to improve maintainability.Suggested fix
Add constants at the package or file level:
const ( metricsPath = "/metrics" hostOperatorMetricsService = "host-operator-metrics-service" )Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` at line 38, Several tests repeatedly use the same string literals for the metrics path and service name; define package/file-level constants (e.g., metricsPath and hostOperatorMetricsService) and replace all occurrences of "/metrics" and "host-operator-metrics-service" (notably in calls to hostAwait.WaitForRouteToBeAvailable and any other checks around metrics) with those constants to remove duplication and improve maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/init.go`:
- Line 218: The code assumes route.Status.Ingress is non-empty when assigning
initHostAwait.MetricsURL (using hostMetricsRoute.Status.Ingress[0].Host) and
similarly for the Logs and Health assignments; add defensive checks before
indexing each route's Ingress slice (for hostMetricsRoute, hostLogsRoute,
hostHealthRoute) to verify len(route.Status.Ingress) > 0 and call t.Fatalf with
a clear message if empty, then proceed to read Ingress[0].Host for the URL
assignments.
---
Outside diff comments:
In `@test/metrics/metrics_test.go`:
- Around line 74-75: The test mistakenly assigns member2Await using
awaitilities.Member1() instead of awaitilities.Member2(), causing both variables
to refer to the same member; update the assignment of member2Await to call
awaitilities.Member2() so the test exercises two distinct members (change the
second line where member2Await is defined).
---
Nitpick comments:
In `@test/metrics/metrics_test.go`:
- Around line 102-107: In the "registration-service" subtest remove the
redundant call to hostAwait.WaitForRouteToBeAvailable that waits for
"host-operator-metrics-service" because the test uses
hostAwait.RegistrationServiceMetricsURL (which is derived from the
"registration-service-metrics" route in init.go) and WaitForDeployments already
guarantees the route is ready; update the subtest to rely on
hostAwait.RegistrationServiceMetricsURL directly and delete the
WaitForRouteToBeAvailable invocation and its error handling.
- Line 38: Several tests repeatedly use the same string literals for the metrics
path and service name; define package/file-level constants (e.g., metricsPath
and hostOperatorMetricsService) and replace all occurrences of "/metrics" and
"host-operator-metrics-service" (notably in calls to
hostAwait.WaitForRouteToBeAvailable and any other checks around metrics) with
those constants to remove duplication and improve maintainability.
In `@testsupport/init.go`:
- Around line 209-245: Extract the duplicated "/metrics" literal into a single
constant (e.g., const metricsPath = "/metrics") at the top of the function or
file, then replace the three usages in the SetupRouteForService calls for
initHostAwait.SetupRouteForService (host metrics),
initHostAwait.SetupRouteForService (registration service metrics), and
initMemberAwait.SetupRouteForService (member metrics) with that constant to
remove duplication and improve maintainability.
In `@testsupport/metrics/metrics.go`:
- Line 45: The HTTP method string in the http.NewRequest call uses "Get"
inconsistent with the canonical uppercase form; update the call in
testsupport/metrics/metrics.go (the http.NewRequest invocation that assigns to
request) to use "GET" instead of "Get" so it matches the other usage (e.g., the
one on line 139) and keeps method casing consistent across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e50c05b-11a2-4f16-bfe6-dd1d9fd37c0d
📒 Files selected for processing (5)
test/metrics/metrics_test.gotestsupport/init.gotestsupport/metrics/metrics.gotestsupport/wait/awaitility.gotestsupport/wait/host.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testsupport/metrics/metrics.go (1)
138-139: Minor inconsistency: HTTP method casing differs from line 45.Line 45 in
getMetricValueuses"Get"while this function uses"GET". While both work (Go'snet/httpnormalizes the method), consider using the canonical uppercase form consistently across both functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/metrics/metrics.go` around lines 138 - 139, The HTTP method string is inconsistent between the two http.NewRequest calls (one uses "Get" in getMetricValue and this one uses "GET"); update the method argument in getMetricValue to the canonical uppercase "GET" (or change this call to match, but prefer "GET") so both http.NewRequest invocations use "GET" consistently; locate the http.NewRequest calls in getMetricValue and the current function and make the method string identical.test/metrics/metrics_test.go (1)
38-39: Extract duplicated service names and paths to constants.The literals
"host-operator-metrics-service"and"/metrics"are repeated multiple times. Consider extracting them to constants at the top of the file or in a shared location.♻️ Suggested constants
const ( hostOperatorMetricsService = "host-operator-metrics-service" registrationServiceMetricsService = "registration-service-metrics" metricsPath = "/metrics" )Also applies to: 106-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 38 - 39, Duplicate string literals for service names and the metrics path in test/metrics/metrics_test.go should be extracted to constants to avoid repetition; add constants (e.g., hostOperatorMetricsService, registrationServiceMetricsService, metricsPath) at the top of the test file and replace all occurrences of "host-operator-metrics-service", "registration-service-metrics" and "/metrics" with those constants where used (for example in calls to hostAwait.WaitForRouteToBeAvailable and similar assertions) so the file references the named constants instead of repeating string literals.testsupport/init.go (1)
209-245: Extract"/metrics"path to a constant to reduce duplication.The literal
"/metrics"is used three times. Extracting it to a constant improves maintainability and satisfies the static analysis hint.♻️ Suggested refactor
Add a constant at the package or function level:
const metricsPath = "/metrics"Then use it in the route setup calls:
- hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics", + hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", metricsPath,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/init.go` around lines 209 - 245, Extract the repeated literal "/metrics" into a single constant (e.g., metricsPath) and use it in all calls to SetupRouteForService to reduce duplication; update the three invocations in this block that call initHostAwait.SetupRouteForService and initMemberAwait.SetupRouteForService (the ones that populate initHostAwait.MetricsURL, initHostAwait.RegistrationServiceMetricsURL, and initMemberAwait.MetricsURL) to pass metricsPath instead of the string literal, and define the constant at the top of the file or function scope where these routes are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/metrics/metrics_test.go`:
- Around line 102-107: The test uses the wrong service name when waiting for the
metrics route: change the call to hostAwait.WaitForRouteToBeAvailable so it
waits for "registration-service-metrics" (the route created in init.go) instead
of "host-operator-metrics-service"; update the invocation in the
"registration-service" t.Run block that currently references hostAwait and
WaitForRouteToBeAvailable so it matches the route used by
hostAwait.RegistrationServiceMetricsURL.
---
Nitpick comments:
In `@test/metrics/metrics_test.go`:
- Around line 38-39: Duplicate string literals for service names and the metrics
path in test/metrics/metrics_test.go should be extracted to constants to avoid
repetition; add constants (e.g., hostOperatorMetricsService,
registrationServiceMetricsService, metricsPath) at the top of the test file and
replace all occurrences of "host-operator-metrics-service",
"registration-service-metrics" and "/metrics" with those constants where used
(for example in calls to hostAwait.WaitForRouteToBeAvailable and similar
assertions) so the file references the named constants instead of repeating
string literals.
In `@testsupport/init.go`:
- Around line 209-245: Extract the repeated literal "/metrics" into a single
constant (e.g., metricsPath) and use it in all calls to SetupRouteForService to
reduce duplication; update the three invocations in this block that call
initHostAwait.SetupRouteForService and initMemberAwait.SetupRouteForService (the
ones that populate initHostAwait.MetricsURL,
initHostAwait.RegistrationServiceMetricsURL, and initMemberAwait.MetricsURL) to
pass metricsPath instead of the string literal, and define the constant at the
top of the file or function scope where these routes are configured.
In `@testsupport/metrics/metrics.go`:
- Around line 138-139: The HTTP method string is inconsistent between the two
http.NewRequest calls (one uses "Get" in getMetricValue and this one uses
"GET"); update the method argument in getMetricValue to the canonical uppercase
"GET" (or change this call to match, but prefer "GET") so both http.NewRequest
invocations use "GET" consistently; locate the http.NewRequest calls in
getMetricValue and the current function and make the method string identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ea3974a-9b41-4ce6-a741-fdd2ccfaf986
📒 Files selected for processing (6)
.govulncheck.yamltest/metrics/metrics_test.gotestsupport/init.gotestsupport/metrics/metrics.gotestsupport/wait/awaitility.gotestsupport/wait/host.go
✅ Files skipped from review due to trivial changes (1)
- .govulncheck.yaml
alexeykazakov
approved these changes
Mar 24, 2026
mfrancisc
reviewed
Mar 25, 2026
Comment on lines
+222
to
+232
| registrationServiceMetricsRoute, err := initHostAwait.SetupRouteForService(t, "registration-service-metrics", "/metrics", | ||
| &routev1.RoutePort{ | ||
| TargetPort: intstr.FromString("regsvc-metrics"), | ||
| }, | ||
| &routev1.TLSConfig{ | ||
| Termination: routev1.TLSTerminationEdge, | ||
| }, | ||
| ) | ||
| require.NoError(t, err) | ||
| initHostAwait.RegistrationServiceMetricsURL = "https://" + registrationServiceMetricsRoute.Status.Ingress[0].Host | ||
| t.Logf("registration service metrics URL: %s", initHostAwait.RegistrationServiceMetricsURL) |
Contributor
There was a problem hiding this comment.
it looks like the route creation is also done in the tests
toolchain-e2e/test/e2e/parallel/registration_service_test.go
Lines 76 to 90 in 629dc7b
Contributor
Author
There was a problem hiding this comment.
ahh, good catch! I only ran the tests in the test/metrics package so I missed the route creation in test/e2e. Let me refactor that
MatousJobanek
approved these changes
Mar 25, 2026
test/metrics/metrics_test.go
Outdated
Comment on lines
+106
to
+107
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | ||
| require.NoError(t, err) |
Collaborator
There was a problem hiding this comment.
the same
Suggested change
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | |
| require.NoError(t, err) |
Contributor
Author
There was a problem hiding this comment.
good point, thanks!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, xcoulon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@xcoulon: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/metrics/metrics_test.go (1)
102-107:⚠️ Potential issue | 🟠 MajorFix route name mismatch in registration-service metrics wait.
Line 106 waits for
registration-service-metrics-service, but the route created intestsupport/init.goisregistration-service-metrics. This can fail the subtest before metrics are queried.🐛 Proposed fix
- _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics-service", "/metrics") + _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics", "/metrics")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 102 - 107, The subtest named "registration-service" calls hostAwait.WaitForRouteToBeAvailable with the wrong route name; replace the hardcoded route string "registration-service-metrics-service" with the actual route name "registration-service-metrics" used by the route created in testsupport/init.go so the call in the registration-service subtest (hostAwait.WaitForRouteToBeAvailable) correctly waits for the existing route.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/metrics/metrics_test.go`:
- Around line 102-107: The subtest named "registration-service" calls
hostAwait.WaitForRouteToBeAvailable with the wrong route name; replace the
hardcoded route string "registration-service-metrics-service" with the actual
route name "registration-service-metrics" used by the route created in
testsupport/init.go so the call in the registration-service subtest
(hostAwait.WaitForRouteToBeAvailable) correctly waits for the existing route.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cab7ee28-7623-4808-8dd8-5dc187dd16c1
📒 Files selected for processing (2)
test/metrics/metrics_test.gotestsupport/init.go
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/parallel/registration_service_test.go (1)
71-90: LGTM with minor suggestion.The refactored test correctly uses
WaitForRouteToBeAvailableand the route name"registration-service-metrics"matches what's created ininit.go.One minor inconsistency: Line 76 uses
route.Spec.HostwhereasWaitForRouteToBeAvailablevalidates usingroute.Status.Ingress[0].Host. While they're typically identical, usingStatus.Ingress[0].Hostwould be more consistent with the function's internal validation.♻️ Optional: Use Status.Ingress[0].Host for consistency
- req, err := http.NewRequest("GET", "http://"+route.Spec.Host+"/metrics", nil) + req, err := http.NewRequest("GET", "http://"+route.Status.Ingress[0].Host+"/metrics", nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/parallel/registration_service_test.go` around lines 71 - 90, The test uses route.Spec.Host to build the metrics URL but WaitForRouteToBeAvailable checks route.Status.Ingress[0].Host, so update the request URL construction to use route.Status.Ingress[0].Host instead of route.Spec.Host to stay consistent with the function's validation (locate the t.Run block that calls WaitForRouteToBeAvailable and the subsequent http.NewRequest creation and replace route.Spec.Host with route.Status.Ingress[0].Host).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/metrics/metrics_test.go`:
- Around line 107-125: The comments in the subtests that call
hostAwait.GetMetricLabels against hostAwait.RegistrationServiceMetricsURL (using
wait.RegistrationServiceCommitMetric and
wait.RegistrationServiceShortCommitMetric) incorrectly mention "Host Operator"
due to copy-paste; update the two comment lines (above the require.Len checks in
the "commit" and "short commit" t.Run blocks) to refer to the
registration-service instead (e.g., "verify that the 'version' metric exists for
registration-service and that it has a non-empty `commit` label"), leaving the
test logic and identifiers (hostAwait.GetMetricLabels,
hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric,
wait.RegistrationServiceShortCommitMetric) unchanged.
- Around line 100-106: The test is waiting for the wrong route name; update the
call to hostAwait.WaitForRouteToBeAvailable in the "registration-service"
subtest to use the actual route name "registration-service-metrics" (replace
"registration-service-metrics-service"), so the WaitForRouteToBeAvailable(t,
hostAwait.Namespace, "registration-service-metrics", "/metrics") invocation
matches the route created in testsupport/init.go; no other behavior needs
changing.
---
Nitpick comments:
In `@test/e2e/parallel/registration_service_test.go`:
- Around line 71-90: The test uses route.Spec.Host to build the metrics URL but
WaitForRouteToBeAvailable checks route.Status.Ingress[0].Host, so update the
request URL construction to use route.Status.Ingress[0].Host instead of
route.Spec.Host to stay consistent with the function's validation (locate the
t.Run block that calls WaitForRouteToBeAvailable and the subsequent
http.NewRequest creation and replace route.Spec.Host with
route.Status.Ingress[0].Host).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fc10d3b-ce51-41ef-9d56-c061040e6a13
📒 Files selected for processing (2)
test/e2e/parallel/registration_service_test.gotest/metrics/metrics_test.go
Comment on lines
+100
to
+106
| t.Run("registration-service", func(t *testing.T) { | ||
|
|
||
| // given | ||
| hostAwait := awaitilities.Host() | ||
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics-service", "/metrics") | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Bug: Route name mismatch will cause test to fail.
Line 104 waits for route "registration-service-metrics-service", but according to testsupport/init.go (lines 223-224), the route is created with name "registration-service-metrics". This test will fail waiting for a non-existent route.
🐛 Proposed fix
- _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics-service", "/metrics")
+ _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics", "/metrics")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("registration-service", func(t *testing.T) { | |
| // given | |
| hostAwait := awaitilities.Host() | |
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics-service", "/metrics") | |
| require.NoError(t, err) | |
| t.Run("registration-service", func(t *testing.T) { | |
| // given | |
| hostAwait := awaitilities.Host() | |
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "registration-service-metrics", "/metrics") | |
| require.NoError(t, err) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/metrics/metrics_test.go` around lines 100 - 106, The test is waiting for
the wrong route name; update the call to hostAwait.WaitForRouteToBeAvailable in
the "registration-service" subtest to use the actual route name
"registration-service-metrics" (replace "registration-service-metrics-service"),
so the WaitForRouteToBeAvailable(t, hostAwait.Namespace,
"registration-service-metrics", "/metrics") invocation matches the route created
in testsupport/init.go; no other behavior needs changing.
Comment on lines
+107
to
+125
| t.Run("commit", func(t *testing.T) { | ||
| // when | ||
| labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric) | ||
|
|
||
| // verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label | ||
| require.Len(t, labels, 1) | ||
| commit := labels[0]["commit"] | ||
| assert.Len(t, commit, len("da3f54634cc65075d51d067a157831d44bf1413e")) // example value: 40 characters | ||
| }) | ||
|
|
||
| t.Run("short commit", func(t *testing.T) { | ||
| // when | ||
| labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceShortCommitMetric) | ||
|
|
||
| // verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label | ||
| require.Len(t, labels, 1) | ||
| commit := labels[0]["commit"] | ||
| assert.Len(t, commit, 7) | ||
| }) |
There was a problem hiding this comment.
Fix misleading comments - these refer to registration-service, not Host Operator.
Lines 111 and 121 contain copy-paste errors from the host-operator subtests. The comments say "Host Operator" but this is the registration-service subtest.
✏️ Proposed fix
t.Run("commit", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric)
- // verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
+ // verify that the "commit" metric exists for registration-service and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, len("da3f54634cc65075d51d067a157831d44bf1413e")) // example value: 40 characters
})
t.Run("short commit", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceShortCommitMetric)
- // verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
+ // verify that the "short_commit" metric exists for registration-service and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, 7)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("commit", func(t *testing.T) { | |
| // when | |
| labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric) | |
| // verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label | |
| require.Len(t, labels, 1) | |
| commit := labels[0]["commit"] | |
| assert.Len(t, commit, len("da3f54634cc65075d51d067a157831d44bf1413e")) // example value: 40 characters | |
| }) | |
| t.Run("short commit", func(t *testing.T) { | |
| // when | |
| labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceShortCommitMetric) | |
| // verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label | |
| require.Len(t, labels, 1) | |
| commit := labels[0]["commit"] | |
| assert.Len(t, commit, 7) | |
| }) | |
| t.Run("commit", func(t *testing.T) { | |
| // when | |
| labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric) | |
| // verify that the "commit" metric exists for registration-service and that it has a non-empty `commit` label | |
| require.Len(t, labels, 1) | |
| commit := labels[0]["commit"] | |
| assert.Len(t, commit, len("da3f54634cc65075d51d067a157831d44bf1413e")) // example value: 40 characters | |
| }) | |
| t.Run("short commit", func(t *testing.T) { | |
| // when | |
| labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceShortCommitMetric) | |
| // verify that the "short_commit" metric exists for registration-service and that it has a non-empty `commit` label | |
| require.Len(t, labels, 1) | |
| commit := labels[0]["commit"] | |
| assert.Len(t, commit, 7) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/metrics/metrics_test.go` around lines 107 - 125, The comments in the
subtests that call hostAwait.GetMetricLabels against
hostAwait.RegistrationServiceMetricsURL (using
wait.RegistrationServiceCommitMetric and
wait.RegistrationServiceShortCommitMetric) incorrectly mention "Host Operator"
due to copy-paste; update the two comment lines (above the require.Len checks in
the "commit" and "short commit" t.Run blocks) to refer to the
registration-service instead (e.g., "verify that the 'version' metric exists for
registration-service and that it has a non-empty `commit` label"), leaving the
test logic and identifiers (hostAwait.GetMetricLabels,
hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric,
wait.RegistrationServiceShortCommitMetric) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
e2e tests for codeready-toolchain/registration-service#588
also, test new
commitandshort_commitmetrics for host and member operatorsSigned-off-by: Xavier Coulon xcoulon@redhat.com
Summary by CodeRabbit
Tests
Chores